Markdown: Modify replace_all to avoid infinite loop #1232
Conversation
|
So... waiting until 2025, when Ubuntu 20.04 is EOL, so can use g_string_replace()? |
|
LGBI but someone needs to test it. The problem is that the markdown plugin has been in and out of plugins due to its delayed GTK3 port so its not clear how many users there are, and its likely to go away again due to wekkit2gtk-4.1 (see #1295 (comment)) unless somebody makes necessary changes. @xiota It needs an active maintainer who can then make decisions like when to swap to new functions even if it makes the plugin not build on older systems, but since you have fixed it without that then yeah probably should wait. |
|
@elextr In a comment somewhere, you mention you're on an LTS distro without access to webkit2gtk-4.1? So I can look up what libraries you have available, what distro and release is it? |
|
IIUC any Ubuntu 20.04 LTS or derivative (eg Linux Mint 20) does not have webkit2gtk-4.1. |
|
So... checking availability of any library on the oldest Ubuntu LTS release should be sufficient? For webkit2gtk, there are a couple discontinued functions that probably need to be updated (in the markdown plugin). Ubuntu 20.04 could be supported with conditional compilation, but if there will be no new geany/plugins release before 20.04 is EOL, there wouldn't be much point. |
|
There were already some mumblings about a point release due to fixes of potential crashes being merged IIUC, and hopefully the next release won't be as far away as 2.0 was from 1.38, but its hard to tell. |
There was a problem hiding this comment.
I extracted the old and new code an tested a bit, and that also works great. FWIW, if somebody didn't catch what the issue was (as in fact it's not mentioned anywhere but "infinite loop"): if the replacement contains the search term, it'll be replaced recursively forever (infinite loop + steady memory growth).
FWIW, I tested using this program:
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <glib.h>
#if 0
/* old */
static void
replace_all(GString *haystack,
const gchar *needle,
const gchar *replacement)
{
gchar *ptr;
gsize needle_len = strlen(needle);
/* For each occurrence of needle in haystack */
while ((ptr = strstr(haystack->str, needle)) != NULL) {
goffset offset = ptr - haystack->str;
g_string_erase(haystack, offset, needle_len);
g_string_insert(haystack, offset, replacement);
}
}
#else
/* new */
static void
replace_all(GString *haystack,
const gchar *needle,
const gchar *replacement)
{
gchar *ptr;
gsize needle_len = strlen(needle);
gsize replacement_len = strlen(replacement);
goffset offset = 0;
/* For each occurrence of needle in haystack */
while ((ptr = strstr(haystack->str + offset, needle)) != NULL) {
offset = ptr - haystack->str;
g_string_erase(haystack, offset, needle_len);
g_string_insert(haystack, offset, replacement);
offset += replacement_len;
}
}
#endif
int main(void)
{
#define TEST_REPLACE_ALL(hs, s, r, ex) \
G_STMT_START { \
GString *haystack_ = g_string_new(hs); \
replace_all(haystack_, s, r); \
g_assert_cmpstr(haystack_->str, ==, ex); \
g_string_free(haystack_, TRUE); \
} G_STMT_END
TEST_REPLACE_ALL("hello", "a", "b", "hello");
TEST_REPLACE_ALL("hallo", "a", "b", "hbllo");
TEST_REPLACE_ALL("foobarbaz", "ba", "BA", "fooBArBAz");
TEST_REPLACE_ALL("ababab", "ba", "BA", "aBABAb");
TEST_REPLACE_ALL("ababab", "ab", "AB", "ABABAB");
TEST_REPLACE_ALL("foobarbaz", "o", "oo", "foooobarbaz");
#undef TEST_REPLACE_ALL
return 0;
}
|
@b4n said:
@xiota said:
Those seem to say the same to me, and "infinite loop" (at least until it runs out of memory :-) sounds like the issue, sounds like furious agreement :-D |
|
Oopsie, I didn't check the OP, only the commit message 😕 |
This PR modifies
replace_allto avoid the infinite loop. In each iteration,replace_allsearches forneedlestarting from the beginning ofhaystack. Ifreplacementcontainsneedle, the result is an infinite loop. To prevent this from happening,replace_allshould continue searching forneedlefrom the end of the previousreplacement.Squashed and rebased from #1128. Resolves #936.